Skip to content

feat: use celery instead of cron jobs for notification digest#38185

Open
AhtishamShahid wants to merge 9 commits intomasterfrom
ahtisham/use_celery_for_notification_digest
Open

feat: use celery instead of cron jobs for notification digest#38185
AhtishamShahid wants to merge 9 commits intomasterfrom
ahtisham/use_celery_for_notification_digest

Conversation

@AhtishamShahid
Copy link
Copy Markdown
Contributor

@AhtishamShahid AhtishamShahid commented Mar 18, 2026

Description

This pull request implements a significant refactor of the email digest scheduling system for notifications. The main change is the shift from using a management command and cron jobs to an automated, deduplicated, and transactional Celery-based scheduling system for daily and weekly digest emails. This ensures that digest emails are reliably scheduled and sent without duplication, even when multiple notifications arrive concurrently. The update introduces a new DigestSchedule model to track scheduled digest tasks, and deprecates the old management command. Additionally, timezone handling has been improved to use Django's timezone utilities.

Issue
#38136

Copilot AI review requested due to automatic review settings March 18, 2026 07:26
@AhtishamShahid AhtishamShahid force-pushed the ahtisham/use_celery_for_notification_digest branch from f9fb0e1 to 08402fe Compare March 18, 2026 07:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors notification digest scheduling from cron/management-command driven execution to Celery-based, deduplicated per-user scheduling backed by a new DigestSchedule model and updated time-window handling.

Changes:

  • Add configurable daily/weekly digest delivery schedule settings and a DigestSchedule model to dedupe scheduled digest tasks.
  • Trigger per-user digest scheduling from send_notifications, and introduce a new delayed Celery task flow for digest delivery.
  • Deprecate the legacy send_email_digest management command and adjust digest date-range calculation to use Django timezone utilities.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
openedx/envs/common.py Adds instance-configurable UTC delivery schedule settings for daily/weekly digests.
openedx/core/djangoapps/notifications/tasks.py Schedules digest delivery when notifications are created for daily/weekly cadence users.
openedx/core/djangoapps/notifications/models.py Introduces DigestSchedule model used as the dedupe source of truth for scheduled digest tasks.
openedx/core/djangoapps/notifications/migrations/0012_digestschedule.py Creates the DigestSchedule table and unique constraint.
openedx/core/djangoapps/notifications/management/commands/send_email_digest.py Marks the cron-driven command as deprecated and changes runtime behavior.
openedx/core/djangoapps/notifications/email/utils.py Switches digest window calculations to django.utils.timezone.now() and returns aware datetimes.
openedx/core/djangoapps/notifications/email/tasks.py Implements new digest scheduling/deduping logic and a per-user delayed Celery task flow.
openedx/core/djangoapps/notifications/email/tests/test_tasks.py Adds/updates tests for the new digest scheduling and delivery behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AhtishamShahid AhtishamShahid marked this pull request as draft March 18, 2026 08:12
@AhtishamShahid AhtishamShahid force-pushed the ahtisham/use_celery_for_notification_digest branch 2 times, most recently from 2be6343 to a33a061 Compare March 24, 2026 07:03
@AhtishamShahid AhtishamShahid marked this pull request as ready for review March 24, 2026 07:06
@AhtishamShahid AhtishamShahid force-pushed the ahtisham/use_celery_for_notification_digest branch 2 times, most recently from 8dee45f to d952ef0 Compare March 25, 2026 08:49
@awais-ansari awais-ansari requested a review from bmtcril March 30, 2026 18:48
Copy link
Copy Markdown
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is looking good, one quick comment. I'm not seeing where this is wired up to Celery, though? Is something external calling send_notifications?

@AhtishamShahid
Copy link
Copy Markdown
Contributor Author

Overall this is looking good, one quick comment. I'm not seeing where this is wired up to Celery, though? Is something external calling send_notifications?

The send notification is called on different signals, and this is handled here

send_notifications.delay(**notification_data)

Copy link
Copy Markdown
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worked for me, but the mechanism is a little hard to understand. Can you update the document to explain the new process and remove the old cron language?

@AhtishamShahid AhtishamShahid force-pushed the ahtisham/use_celery_for_notification_digest branch from b252abb to 29d10c4 Compare April 9, 2026 09:52
Copy link
Copy Markdown
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all of the work on this, it worked for me locally 👍 👍

@AhtishamShahid
Copy link
Copy Markdown
Contributor Author

This worked for me, but the mechanism is a little hard to understand. Can you update the document to explain the new process and remove the old cron language?

Added docs and removed references for cron

@bmtcril
Copy link
Copy Markdown
Contributor

bmtcril commented Apr 9, 2026

Thanks, this should be good to go 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants